crates/recording/src/sources/screen_capture/mod.rs (1)

197-203: LGTM! Clean implementation.

The method correctly returns string representations for all ScreenCaptureTarget variants. The implementation is straightforward and appropriate for providing human-readable labels.

Optional: Consider adding unit tests.

Since this is a new public method, you may want to add a simple test to verify the string representations:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_kind_str() {
        let display = ScreenCaptureTarget::Display { id: DisplayId::default() };
        assert_eq!(display.kind_str(), "Display");

        let window = ScreenCaptureTarget::Window { id: WindowId::default() };
        assert_eq!(window.kind_str(), "Window");

        let area = ScreenCaptureTarget::Area {
            screen: DisplayId::default(),
            bounds: LogicalBounds::new(LogicalPosition::new(0.0, 0.0), LogicalSize::new(100.0, 100.0)),
        };
        assert_eq!(area.kind_str(), "Area");
    }
}

apps/desktop/src/routes/(window-chrome)/settings/general.tsx (2)

653-660: Consider relaxing the minimum length constraint.

The isSaveDisabled function prevents saving templates with length <= 3, which might be too restrictive. Valid templates like "{D}" (day of month) have only 3 characters but are legitimate use cases.

Consider removing or reducing the minimum length check:

 const isSaveDisabled = () => {
   const input = inputValue();
   return (
     !input ||
-    input === (props.value ?? DEFAULT_PROJECT_NAME_TEMPLATE) ||
-    input.length <= 3
+    input === (props.value ?? DEFAULT_PROJECT_NAME_TEMPLATE)
   );
 };

Alternatively, validate that the template contains at least one placeholder instead of checking raw length.

---

735-741: Consider adding blur animation to Collapsible for smoother transitions.

The PR description mentions adding blur to the collapsible animation. The current implementation uses opacity transitions, but you may want to add a blur effect for enhanced visual polish, consistent with the PR objectives.

Add blur to the animation if desired:

-<Collapsible.Content class="opacity-0 transition animate-collapsible-up ui-expanded:animate-collapsible-down ui-expanded:opacity-100 text-xs text-gray-12 space-y-3 px-1 pb-2">
+<Collapsible.Content class="opacity-0 blur-sm transition animate-collapsible-up ui-expanded:animate-collapsible-down ui-expanded:opacity-100 ui-expanded:blur-0 text-xs text-gray-12 space-y-3 px-1 pb-2">

crates/utils/src/lib.rs (1)

200-222: Consider expanding test coverage for edge cases.

The current test validates basic conversion and the borrowed case, but could benefit from additional test cases to ensure robustness.

Add tests for additional scenarios:

#[test]
fn test_moment_format_edge_cases() {
    // Test overlapping patterns
    assert_eq!(moment_format_to_chrono("MMMMM"), "%BM");
    
    // Test partial matches
    assert_eq!(moment_format_to_chrono("YYY"), "YY%y");
    
    // Test all placeholder types
    assert_eq!(moment_format_to_chrono("hh:mm:ss A"), "%I:%M:%S %p");
    assert_eq!(moment_format_to_chrono("HH:mm:ss"), "%H:%M:%S");
    
    // Test no-op with special characters
    assert_eq!(moment_format_to_chrono("---///"), "---///");
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to **/tauri.ts : Do not edit auto-generated files named `tauri.ts`.

Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to packages/ui/**/*.{ts,tsx,js,jsx} : Component files in `packages/ui` should use PascalCase naming if they define React/Solid components.

apps/desktop/src-tauri/src/recording.rs (1)

263-324: The {target} placeholder concern appears addressed.

The past review comment flagged that {target} was mentioned in documentation but not implemented. The current documentation and example (line 290) correctly use {target_kind} and {target_name} as separate placeholders, which are implemented in the AhoCorasick patterns (lines 317-320). This appears to resolve the previous concern.

apps/desktop/src-tauri/src/recording.rs (2)

24-24: Consider using std::sync::LazyLock for consistency.

The codebase already uses LazyLock in moment_format_to_chrono (from the relevant code snippets). For consistency and to use the more modern standard library feature (stable since Rust 1.80), consider replacing lazy_static! with LazyLock in the format_project_name function.

---

405-406: Consider removing redundant colon replacement.

The sanitize_filename::sanitize() call on line 406 should already handle filesystem-unsafe characters including colons. The explicit .replace(":", ".") on line 405 appears redundant, though it's harmless.

If you prefer to keep the explicit replacement for clarity, this is fine. Otherwise, you can simplify to:

-    let filename = project_name.replace(":", ".");
-    let filename = format!("{}.cap", sanitize_filename::sanitize(&filename));
+    let filename = format!("{}.cap", sanitize_filename::sanitize(&project_name));

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/desktop/src-tauri/src/recording.rs (1)

263-377: Consider using LazyLock for consistency and adding unit tests.

The function correctly implements template formatting with multiple placeholder types. Two improvements to consider:

1. Consistency with utils crate: The cap_utils crate uses std::sync::LazyLock (see moment_format_to_chrono), but this function uses lazy_static!. For consistency, consider migrating to LazyLock:

-use lazy_static::lazy_static;
+use std::sync::LazyLock;

-    lazy_static! {
-        static ref DATE_REGEX: Regex = Regex::new(r"\{date(?::([^}]+))?\}").unwrap();
-        static ref TIME_REGEX: Regex = Regex::new(r"\{time(?::([^}]+))?\}").unwrap();
-        static ref MOMENT_REGEX: Regex = Regex::new(r"\{moment(?::([^}]+))?\}").unwrap();
-        static ref AC: aho_corasick::AhoCorasick = {
+    static DATE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{date(?::([^}]+))?\}").unwrap());
+    static TIME_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{time(?::([^}]+))?\}").unwrap());
+    static MOMENT_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{moment(?::([^}]+))?\}").unwrap());
+    static AC: LazyLock<aho_corasick::AhoCorasick> = LazyLock::new(|| {
             aho_corasick::AhoCorasick::new(&[
                 "{recording_mode}",
                 "{mode}",
                 "{target_kind}",
                 "{target_name}",
             ])
             .expect("Failed to build AhoCorasick automaton")
-        };
-    }
+    });

2. Unit test coverage: This function has multiple code paths (different placeholders, custom format specifiers, edge cases). Consider adding unit tests in the tests module to verify behavior with various templates, format specifiers, and edge cases.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/desktop/src-tauri/Cargo.toml (1)

114-114: Clarify the commented feature syntax on line 114.

Line 114 has trailing commented-out code that appears to be incomplete feature specifications. This may confuse future maintainers. Consider either removing this comment or clarifying its intent.

-opentelemetry-otlp = "0.31.0"                                                 #{ version = , features = ["http-proto", "reqwest-client"] }
+opentelemetry-otlp = "0.31.0"

apps/desktop/src-tauri/src/recording.rs (3)

24-24: Consider using std::sync::LazyLock for consistency.

The lazy_static crate is used here, but the cap_utils crate uses std::sync::LazyLock (stable since Rust 1.80). Using LazyLock would improve consistency across the codebase and reduce external dependencies.

Apply this diff to use LazyLock:

-use lazy_static::lazy_static;
+use std::sync::LazyLock;

Then update the lazy initialization in format_project_name (around line 314):

-    lazy_static! {
-        static ref DATE_REGEX: Regex = Regex::new(r"\{date(?::([^}]+))?\}").unwrap();
-        static ref TIME_REGEX: Regex = Regex::new(r"\{time(?::([^}]+))?\}").unwrap();
-        static ref MOMENT_REGEX: Regex = Regex::new(r"\{moment(?::([^}]+))?\}").unwrap();
-        static ref AC: aho_corasick::AhoCorasick = {
+    static DATE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{date(?::([^}]+))?\}").unwrap());
+    static TIME_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{time(?::([^}]+))?\}").unwrap());
+    static MOMENT_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"\{moment(?::([^}]+))?\}").unwrap());
+    static AC: LazyLock<aho_corasick::AhoCorasick> = LazyLock::new(|| {
             aho_corasick::AhoCorasick::new(&[
                 "{recording_mode}",
                 "{mode}",
                 "{target_kind}",
                 "{target_name}",
             ])
             .expect("Failed to build AhoCorasick automaton")
-        };
-    }
+    });

Based on learnings

---

263-377: Consider adding unit tests for format_project_name.

This is a core public function exposed as a Tauri command with complex formatting logic involving multiple token replacements. Unit tests would help ensure correctness and prevent regressions, especially for edge cases like:

• Empty or missing template
• Custom date/time formats with moment syntax
• All placeholder combinations
• Special characters in target names

Example test structure:

#[cfg(test)]
mod format_project_name_tests {
    use super::*;
    use chrono::TimeZone;

    #[test]
    fn test_default_template() {
        let dt = chrono::Local.with_ymd_and_hms(2025, 11, 12, 15, 23, 0).unwrap();
        let result = format_project_name(
            None,
            "Built-in Retina Display",
            "Display",
            RecordingMode::Instant,
            Some(dt),
        );
        assert!(result.contains("Built-in Retina Display"));
        assert!(result.contains("Display"));
        assert!(result.contains("2025-11-12"));
    }

    #[test]
    fn test_custom_moment_format() {
        let dt = chrono::Local.with_ymd_and_hms(2025, 11, 12, 15, 23, 0).unwrap();
        let result = format_project_name(
            Some("{moment:YYYY-MM-DD HH:mm}"),
            "Test",
            "Window",
            RecordingMode::Studio,
            Some(dt),
        );
        assert!(result.contains("2025-11-12 15:23"));
    }

    #[test]
    fn test_all_placeholders() {
        let result = format_project_name(
            Some("{recording_mode} {mode} {target_kind} {target_name}"),
            "Chrome",
            "Window",
            RecordingMode::Studio,
            None,
        );
        assert_eq!(result, "Studio studio Window Chrome");
    }
}

---

593-593: Consider renaming target_name field to project_name for clarity.

The target_name field in InProgressRecordingCommon is now populated with the formatted project_name rather than the raw capture target name. This semantic mismatch could confuse future maintainers. While this works functionally, renaming the field to project_name throughout the structs (InProgressRecordingCommon, CompletedRecording) would better reflect its actual purpose.

Note: This would be a broader refactor touching multiple files and is not urgent.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)

105-107: Keep TS and Rust default templates in sync

DEFAULT_PROJECT_NAME_TEMPLATE mirrors the Rust DEFAULT_FILENAME_TEMPLATE in recording::format_project_name. To avoid future drift, consider adding a brief comment referencing the Rust constant (or a test that asserts both defaults format the same example) so changes on one side don’t silently desync.

apps/desktop/src-tauri/src/recording.rs (1)

720-724: Field name target_name now holds project name

InProgressRecordingCommon { target_name: project_name, .. } repurposes the target_name field to actually carry the formatted project name. That’s fine behaviorally (the field is only used to plumb a display name through CompletedRecording), but the naming is now misleading.

If you touch this area again, consider renaming the field (and CompletedRecording::target_name) to something like project_name to better reflect its semantics; no urgent change needed.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/desktop/src-tauri/src/update_project_names.rs (2)

27-34: Critical: Migration flag still set on failure (past issue remains)

The past review comment correctly identified that lines 27-34 mark the migration as complete even when migrate returns an error. This means:

• If migration fails for any reason (permissions, conflicts, etc.), the flag is still set
• Future app launches will never retry the migration
• Users will permanently have UUID-based folders

---

100-138: Major: Partial failures still swallowed (past issue remains)

The past review correctly identified that this function returns Ok(()) even when failed > 0. Combined with the flag-setting issue in migrate_if_needed, a single failure permanently blocks future migration attempts.

The detailed performance logging (wall time, averages, top slowest projects) is excellent for observability.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

crates/utils/src/lib.rs (2)

45-100: Clarify failure behavior and validate attempts in ensure_unique_filename*_with_attempts.

The core logic looks solid, but a couple of edge cases are underspecified:

• ensure_unique_filename_with_attempts accepts any i32 for attempts. For attempts <= 0, the loop will error immediately (after at most one probe) with "Too many filename conflicts", even when there are no real conflicts.
• The doc comment on ensure_unique_filename promises “Returns the unique filename that doesn't conflict with existing files” but doesn’t mention that the function can fail when the suffix limit is exceeded.

Consider tightening this by:

• Validating attempts at the top (e.g., debug_assert!(attempts > 0) or returning an explicit error if attempts <= 0), and
• Updating the docs to describe the error case (and that attempts bounds the highest numeric suffix tried).

Optionally, if base_filename is always expected to be a bare file name (no separators), a debug_assert!(!base_filename.contains(std::path::MAIN_SEPARATOR)) would encode that assumption.

---

209-231: Extend tests to cover documented edge cases (e.g., ISO week and invalid/unknown tokens).

The current test nicely exercises a mixed format and the borrowed/unchanged path. Given the richer doc examples, you might want to add:

• A test for the ISO week example (once the implementation or docs are corrected) so the mismatch can’t regress silently.
• One or two tests around “near misses” (e.g., strings containing partial tokens or literals adjacent to tokens) to lock in the LeftmostLongest behavior.

This would make the behavior of moment_format_to_chrono much clearer and future refactors safer.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

crates/utils/src/lib.rs (1)

114-182: Address moment.js compatibility confusion flagged in previous review.

The previous review flagged that this function claims to convert "moment template format" but uses incompatible token semantics. This concern was not fully addressed:

• Moment.js convention: DDD/DDDD = day-of-year (1-365, 001-365); ddd/dddd = weekday names
• This implementation: DDD/DDDD = weekday names; day-of-year tokens are unsupported

The current documentation still refers to "moment template format" (line 114) without clarifying this is a custom subset with different semantics. Users expecting moment.js compatibility will encounter unexpected behavior.

Based on previous review comments, either:

1. Update the function description and docs to explicitly state this is a "moment-style subset" with a clear notice about the divergence:

-/// Converts user-friendly moment template format strings to chrono format strings.
+/// Converts moment-style template format strings to chrono format strings.
 ///
-/// This function translates common template format patterns to chrono format specifiers,
-/// allowing users to write intuitive date/time formats that get converted to chrono's format.
+/// This function translates a custom subset of date/time patterns to chrono format specifiers.
+/// 
+/// **Note**: This is NOT fully compatible with moment.js. Notably, `DDD`/`DDDD` map to 
+/// weekday names here, whereas in moment.js they represent day-of-year. Day-of-year and 
+/// ISO week tokens are not supported.

2. Or realign the tokens to match moment.js if compatibility is important (add ddd/dddd for weekdays, repurpose DDD/DDDD for day-of-year).

apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)

9-9: Default project name card wiring looks solid; consider debouncing preview updates

The new DefaultProjectNameCard is well-integrated: the DEFAULT_PROJECT_NAME_TEMPLATE fallback, value/onChange wiring into defaultProjectNameTemplate, Save/Reset behavior, and the help section with copyable placeholders all read correctly, and the Save handler now correctly awaits persistence and preview updates.

One remaining improvement is avoiding a Tauri round-trip on every keystroke in onInput via updatePreview(e.currentTarget.value). A small trailing-edge debounce (e.g., 200–300ms) would reduce redundant formatProjectName calls for fast typers and also mitigate the chance of a slower earlier call overwriting a newer preview.

For example:

 function DefaultProjectNameCard(props: {
@@
-	const [momentExample, setMomentExample] = createSignal("");
+	const [momentExample, setMomentExample] = createSignal("");
+	let previewTimeoutId: number | undefined;
@@
-	async function updatePreview(val = inputValue()) {
+	async function updatePreview(val = inputValue()) {
 		const formatted = await commands.formatProjectName(
 			val,
@@
 		setPreview(formatted);
 	}
+
+	function schedulePreviewUpdate(val: string) {
+		if (previewTimeoutId !== undefined) {
+			clearTimeout(previewTimeoutId);
+		}
+		previewTimeoutId = window.setTimeout(() => {
+			// Best-effort; log if something goes wrong instead of throwing in the UI event loop.
+			void updatePreview(val).catch((error) => {
+				console.error("Failed to update project name preview", error);
+			});
+		}, 250);
+	}
@@
-					onInput={(e) => {
-						setInputValue(e.currentTarget.value);
-						updatePreview(e.currentTarget.value);
-					}}
+					onInput={(e) => {
+						const next = e.currentTarget.value;
+						setInputValue(next);
+						schedulePreviewUpdate(next);
+					}}

This keeps the explicit await updatePreview(...) calls for Reset/Save semantics while making the live preview cheaper and more robust during typing.

Also applies to: 105-107, 546-551, 638-838

crates/utils/src/lib.rs (1)

217-239: Consider expanding test coverage.

The current tests only cover moment_format_to_chrono with basic scenarios. Consider adding:

• Tests for ensure_unique_filename and ensure_unique_filename_with_attempts to verify:
  • Behavior when file doesn't exist (returns base filename)
  • Conflict resolution (appending numbers)
  • Extension handling (files with/without extensions)
  • Max attempts limit enforcement
• Additional moment_format_to_chrono tests for:
  • Overlapping pattern handling (e.g., "MMMM" vs "MMM" precedence)
  • Mixed token and literal text

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/desktop/src-tauri/src/lib.rs (1)

3062-3078: LGTM! Command implementation is correct.

The command properly delegates to recording::format_project_name with correct parameter type conversions. The async marker is unnecessary since no async operations are performed, but this is a minor inefficiency and may be intentional for consistency with other commands.

If desired, you can remove the async marker for a slight performance improvement:

-#[tauri::command]
-#[specta::specta]
-async fn format_project_name(
+#[tauri::command(async)]
+#[specta::specta]
+fn format_project_name(
     template: Option<String>,
     target_name: String,
     target_kind: String,
     recording_mode: RecordingMode,
     datetime: Option<chrono::DateTime<chrono::Local>>,
 ) -> String {

Note: The #[tauri::command(async)] attribute makes the command callable asynchronously from the frontend while keeping the Rust implementation synchronous.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro